Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Replaced EntityMap with HashMap #9461

Merged
merged 5 commits into from
Aug 28, 2023
Merged

Replaced EntityMap with HashMap #9461

merged 5 commits into from
Aug 28, 2023
+90 −139

Conversation

bushrat011899
Copy link
Contributor

@bushrat011899 bushrat011899 commented Aug 17, 2023 •
edited by alice-i-cecile
Loading

Objective

Solution

  • EntityMap has been replaced by a simple HashMap<Entity, Entity>.

Changelog

  • EntityMap::world_scope has been replaced with World::world_scope to avoid creating a new trait. This is a public facing change to the call semantics, but has no effect on results or behaviour.
  • EntityMap, as a HashMap, now operates on &Entity rather than Entity. This changes many standard access functions (e.g, .get) in a public-facing way.

Migration Guide

  • Calls to EntityMap::world_scope can be directly replaced with the following:
    map.world_scope(&mut world) -> world.world_scope(&mut map)
  • Calls to legacy EntityMap methods such as EntityMap::get must explicitly include de/reference symbols:
    let entity = map.get(parent); -> let &entity = map.get(&parent);

@bushrat011899
Replaced EntityMap with HashMap
9834d74 
Minimum changes required to replace the `EntityMap` structure with a type-alias of `HashMap`.

`EntityMap::world_scope` has been moved replaced with `World::world_scope` to avoid creating a new trait.

`HashMap` works on `&Entity` rather than `Entity` like the original `EntityMap`, which does change the public API in a minimal way.
@github-actions
Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@bushrat011899
Copy link
Contributor Author

Hi! This is my first contribution to Bevy. I've read through the contributions document and ensured all CI tests pass locally on my machine. This pull request would Close #9357. Please let me know if there's anything else I can do to help here!

@github-actions
Copy link
Contributor

Example alien_cake_addict failed to run, please try running it locally and check the result.

@bushrat011899
Copy link
Contributor Author

Just ran the alien_cake_addict example locally and I did not experience any issues. I think this is a false positive, but I am on Windows so perhaps there's a deeper interaction I'm unaware of.

The check-compiles CI test that is failing also appears to be a false-positive, as it's a failure test that is still failing, just with a slightly different error message than I saw on my machine (same error code, different explanation text)

@bushrat011899
Update missing_deref.fail.stderr
281833b 
Reverted change caused by running CI on Windows 10. Running CI on Ubuntu through WSL produces more consistent behaviour.
@bushrat011899
Copy link
Contributor Author

Example alien_cake_addict failed to run, please try running it locally and check the result.

Confirmed alien_cake_addict runs under WSLg on Ubuntu, and reverted my change which failed check-compiles on Ubuntu CI.

@github-actions
Copy link
Contributor

Example alien_cake_addict failed to run, please try running it locally and check the result.

@nicopap
Copy link
Contributor

nicopap commented Aug 17, 2023

CI is currently broken, don't worry about the bot messages.

@bushrat011899
Copy link
Contributor Author

CI is currently broken, don't worry about the bot messages.

Glad I'm not insane! Thanks!

@nicopap nicopap mentioned this pull request Aug 17, 2023
Closed
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Aug 17, 2023
alice-i-cecile
alice-i-cecile approved these changes Aug 17, 2023
View reviewed changes
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like how much this simplifies this code.

I'm a bit opposed to keeping around the type-alias, as I think it just obscures what's going on at this point. But I'll defer to you and the other reviewers there. The docs are nice, so if we remove it perhaps we can migrate the meaty bits to module docs for map_entities.rs.

@alice-i-cecile alice-i-cecile added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Aug 17, 2023
Shatur
Shatur suggested changes Aug 17, 2023
View reviewed changes
crates/bevy_ecs/src/world/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/entity/map_entities.rs Outdated Show resolved Hide resolved
@bushrat011899
Removed EntityMap Type Alias
888d549 
Removed the EntityMap type alias and moved the world_scope method onto EntityMapper.
@github-actions
Copy link
Contributor

Example alien_cake_addict failed to run, please try running it locally and check the result.

Shatur
Shatur suggested changes Aug 17, 2023
View reviewed changes
crates/bevy_scene/src/serde.rs Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

Example alien_cake_addict failed to run, please try running it locally and check the result.

@github-actions
Copy link
Contributor

Example alien_cake_addict failed to run, please try running it locally and check the result.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Aug 18, 2023
@nicopap
Copy link
Contributor

nicopap commented Aug 18, 2023 •
edited
Loading

What's the benefit of this over making the map field pub? Just make the field pub and remove the delegation methods. Newtyping is useful to not shoot yourself in the foot.

@Shatur
Copy link
Contributor

Shatur commented Aug 18, 2023 •
edited
Loading

What's the benefit of this over making the map field pub? Just make the field pub and remove the delegation methods.

Because it's not used as a component, so you can't shoot yourself in the foot. This is why I suggested this approach over newtype.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Aug 28, 2023
Merged via the queue into bevyengine:main with commit 394e2b0 Aug 28, 2023
@nicopap
Copy link
Contributor

nicopap commented Sep 5, 2023

I know this is already merged, but

Because it's not used as a component,

It's not really an argument against newtyping. newtypes is used as a pattern outside of ECS very widely, especially in the rust community. It's used to add semantic meaning. Even if the inner field is public. For example, here, you know it's specifically an map meant to be used for entity mapping, not any kind of Entity to Entity map (say a relation map). You can't feed an unrelated variable accidentally.

Though having to use the API now, it feels silly to newtype specifically this.

rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
@bushrat011899
Replaced EntityMap with HashMap (bevyengine#9461)
9111114 
# Objective

- Fixes bevyengine#9321

## Solution

- `EntityMap` has been replaced by a simple `HashMap<Entity, Entity>`.

---

## Changelog

- `EntityMap::world_scope` has been replaced with `World::world_scope`
to avoid creating a new trait. This is a public facing change to the
call semantics, but has no effect on results or behaviour.
- `EntityMap`, as a `HashMap`, now operates on `&Entity` rather than
`Entity`. This changes many standard access functions (e.g, `.get`) in a
public-facing way.

## Migration Guide

- Calls to `EntityMap::world_scope` can be directly replaced with the
following:
  `map.world_scope(&mut world)` -> `world.world_scope(&mut map)`
- Calls to legacy `EntityMap` methods such as `EntityMap::get` must
explicitly include de/reference symbols:
  `let entity = map.get(parent);` -> `let &entity = map.get(&parent);`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Selene-Amanita Selene-Amanita Selene-Amanita approved these changes

@Shatur Shatur Shatur approved these changes

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@mockersf mockersf Awaiting requested review from mockersf

Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Replace EntityMap with regular HashMap
5 participants
@bushrat011899 @nicopap @Shatur @alice-i-cecile @Selene-Amanita